Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.23%. Comparing base (04b8ee7) to head (11c8a50).
Report is 1 commits behind head on main.

@@             Coverage Diff              @@
##               main    #1789      +/-   ##
============================================
+ Coverage     92.22%   92.23%   +0.01%     
  Complexity     1817     1817              
============================================
  Files           121      121              
  Lines          6813     6823      +10     
============================================
+ Hits           6283     6293      +10     
  Misses          530      530              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Try the dev build for this PR: https://acquia-cli.s3.amazonaws.com/build/pr/1789/acli.phar

curl -OL https://acquia-cli.s3.amazonaws.com/build/pr/1789/acli.phar
chmod +x acli.phar

It looks like datastore support for Git URLs was added in #733 but we weren't doing schema validation of the yaml store at that time. When we added the config tree validator I'm not sure why tests kept passing 🤷

This looks good but we need to figure out why tests didn't catch this to prevent it from regressing again.

Let me know if that's something you can figure out, or we can try to get this into an upcoming sprint.

I think command failing and test not failing because -

For command when runs, container builds and thus acsf command factory or api command factory loads and loads other services - https://github.com/acquia/cli/blob/main/src/Command/Acsf/AcsfCommandFactory.php#L41 which loads the dataStore service. this further loads yamlstore service and that calls https://github.com/acquia/cli/blob/main/src/DataStore/Datastore.php#L69 and there this processConfiguration internally calls the treebuilder to validate the tree

and given in project repo, this acli config file already exists and thus validation fails while building the tree on container load / initialize

In test run, this acli data store service is created from https://github.com/acquia/cli/blob/main/tests/phpunit/src/TestBase.php#L820 and as there is no acli config file exists, this passes and then there is no schema validation is done when something is set - https://github.com/acquia/cli/blob/main/tests/phpunit/src/Commands/Push/PushArtifactCommandTest.php#L128

SO in https://github.com/acquia/cli/blob/main/tests/phpunit/src/TestBase.php#L75, I changed this to

protected array $acliConfig = [
        'cloud_app_uuid' => 'some-app-id',
        'push' => [
            'artifact' => [
                'destination-git-urls' => [
                    'aa',
                    'bb',
                ]
            ],
        ],
    ];

and test failed . This is called from https://github.com/acquia/cli/blob/main/tests/phpunit/src/TestBase.php#L428

PHPUnit 9.6.20 by Sebastian Bergmann and contributors.

Test 'Acquia\Cli\Tests\Commands\Push\PushArtifactCommandTest::testPushArtifactWithAcquiaCliFile' started
Test 'Acquia\Cli\Tests\Commands\Push\PushArtifactCommandTest::testPushArtifactWithAcquiaCliFile' ended


Time: 00:00.032, Memory: 28.00 MB

There was 1 error:

1) Acquia\Cli\Tests\Commands\Push\PushArtifactCommandTest::testPushArtifactWithAcquiaCliFile
Acquia\Cli\Exception\AcquiaCliException: Configuration file at the following path contains invalid keys: vfs://root/project/.acquia-cli.yml Unrecognized option "push" under "acquia_cli". Available option is "cloud_app_uuid".

@joshirohit100 thanks a lot for diving into that, I think you're right, the problem is we used acliDatastore->set() to establish test fixtures and this bypassed validation. We should have written the datastores as files and then reloaded the datstore/command to trigger validation. I'll fix that in #1796